DEV-14971: Python SDK storage-service compatibility unit tests#459
DEV-14971: Python SDK storage-service compatibility unit tests#459
Conversation
…ervice response shapes Audit confirms no client patches needed — storage-service produces the same response shapes Rainbow does: - /files/store: path/name/upload_type match what UploadDocument expects - indico-file:// URI construction/round-trip via CreateStorageURLs works - RetrieveStorageObject strips indico-file:// prefix correctly Tests mock at the HTTP level; no running service required. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sihrc
requested review from
Sung96kim,
arsandhu,
goatrocks,
jacobmanderson and
nicholas-lockhart
|
Storage migration freeze checkpoint (2026-04-15): PR remains under NOT MERGE policy, but is still failing from 2026-04-12. Requesting a CI rerun to refresh review evidence while keeping this PR unmerged. |
|
Correction: failing check context is publish_python_sdk-unit_tests. Please rerun this CI context for fresh freeze-period validation signal (PR remains NOT MERGE). |
|
Escalation: check context publish_python_sdk-unit_tests is still stale (last started 2026-04-12T00:02:54Z) after rerun request. Requesting CI run owners/maintainers to trigger a fresh Harness run so we can refresh freeze-period validation evidence. PR remains NOT MERGE. |
|
@goatrocks @Sung96kim @jacobmanderson @nicholas-lockhart @arsandhu and @IndicoDataSolutions/pr-be-indicodata-ai: targeted rerun request for stale Harness context publish_python_sdk-unit_tests (still on 2026-04-12T00:02:54Z). Please trigger/own a fresh run for freeze-period validation evidence. PR remains NOT MERGE. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| uri = "indico-file:///storage/uploads/42/abc-uuid" | ||
| req = RetrieveStorageObject(uri) | ||
| assert req.path == "/storage/uploads/42/abc-uuid" | ||
| assert req.method == HTTPMethod.GET |
There was a problem hiding this comment.
Test requests unused mock_request and client fixtures
Low Severity
test_create_storage_urls_round_trips_through_retrieve declares mock_request and client as parameters but never references either in the test body. These fixtures trigger HTTP client initialization (including an auth token refresh mock), adding unnecessary setup overhead and coupling. Compare with test_retrieve_storage_object_strips_indico_file_scheme and test_retrieve_storage_object_accepts_dict_with_url_key, which correctly omit fixtures they don't need.
Reviewed by Cursor Bugbot for commit 00d3720. Configure here.
jacobmanderson
left a comment
jacobmanderson
left a comment
There was a problem hiding this comment.
Blocking on the failing code-check path: the new storage tests depend on an ambient API token, so a clean tox run fails before the mocked storage calls execute. I reproduced the failure with uv run --extra test tox -e py313; the narrow uv run pytest path only passed because the ambient environment was more permissive.
- Jacob's AI Assistant
|
|
||
| @pytest.fixture | ||
| def cfg(): | ||
| return IndicoConfig(protocol="mock", host="mock") |
There was a problem hiding this comment.
These new storage tests still let IndicoConfig resolve an API token from the environment or ~/indico_api_token.txt. In a clean tox/code-check environment that does not provide INDICO_API_TOKEN, setup fails before the HTTP mocks run, which matches the currently failing publish_python_sdk-code_checks signal. Please pass a dummy token in the test configs, e.g. IndicoConfig(protocol="mock", host="mock", api_token="test-token"); the inline redirect test at line 237 needs the same treatment.
- Jacob's AI Assistant
|
Ran a clean local reproduction of the previously failing path on latest head ():\n\n- .pkg: _optional_hooks> python /home/chris/indico/indico-client-python/.venv/lib/python3.12/site-packages/pyproject_api/_backend.py True hatchling.build tests/unit/test_filters.py::test_filter PASSED =============================== warnings summary =============================== -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html |
|
Verification on latest head
I pushed |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a86cbd6. Configure here.
|
|
||
| @pytest.fixture | ||
| def cfg(): | ||
| return IndicoConfig(protocol="mock", host="mock") |
There was a problem hiding this comment.
Missing api_token causes RuntimeError in CI environments
High Severity
The cfg fixture and the inline IndicoConfig in test_retrieve_storage_object_follows_redirects both omit api_token. When INDICO_API_TOKEN is not set in the environment, IndicoConfig.__init__ calls _resolve_api_token(), which tries to read ~/indico_api_token.txt and raises RuntimeError if absent. This causes all tests depending on cfg (and the redirect test) to fail in clean CI environments before any HTTP mocks run. Passing a dummy token like api_token="test-token" avoids the filesystem lookup.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a86cbd6. Configure here.
|
Pushed follow-up commit 92bd191 to address a reproducible code-check blocker:\n\n- Fixed 124 files already formatted violations in and .\n- Re-ran locally on the branch:\n - 104 files already formatted ✅\n - .pkg: _optional_hooks> python /home/chris/indico/indico-client-python/.venv/lib/python3.12/site-packages/pyproject_api/_backend.py True hatchling.build tests/unit/test_filters.py::test_filter PASSED =============================== warnings summary =============================== -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html |
|
Pushed follow-up commit
Current gate state after push: PR checks are re-running on the new head commit. |
2 participants
Summary
/files/storereturnspath/name/upload_typematching exactly whatUploadDocumentexpects;indico-file://URI handling is unchangedUploadDocument,CreateStorageURLs, andRetrieveStorageObjectagainst theLegacyUploadResponseItemresponse shapeTest plan
test_upload_document_posts_to_storage_files_store— confirms POST target pathtest_upload_document_processes_path_name_upload_type— confirms response field mappingtest_upload_document_handles_multiple_files— multi-file upload batchtest_create_storage_urls_builds_indico_file_uris— URI construction from responsetest_create_storage_urls_round_trips_through_retrieve— round-trip URI → pathtest_retrieve_storage_object_strips_indico_file_scheme— path extractiontest_retrieve_storage_object_accepts_dict_with_url_key— dict input varianttest_retrieve_storage_object_fetches_content— end-to-end GET with mock responseAll 8 tests pass. No network required.
Part of DEV-14699 storage-service migration epic.
🤖 Generated with Claude Code
Note
Low Risk
Adds new test coverage and adjusts dev/test dependencies; production SDK behavior is unchanged, with low risk aside from potential CI/dependency resolution issues.
Overview
Adds unit tests to validate Python SDK compatibility with the storage-service (Rainbow replacement) response shapes, covering
UploadDocument,CreateStorageURLs,RetrieveStorageObject(including redirect handling), and signed-URL upload behavior in_UploadSMExport.Adds a small unit test suite to ensure the
uv-dynamic-versioningrelease tag regex inpyproject.tomlmatches standard, prerelease, and post-release tags.Updates dev/test tooling: expands
[dependency-groups].dev(addspytest-asyncio,requests-mock,msgpack), addstomlifor Python <3.11 intox.ini, and refreshesuv.lock; includes a minor assertion formatting tweak intests/integration/queries/test_gallery.py.Reviewed by Cursor Bugbot for commit 92bd191. Bugbot is set up for automated code reviews on this repo. Configure here.